What happened with the authenticated route?

I changed it to RouteItem, it's more generic. That component has the potential to receive any condition for being shown or not. For instance, if you have many user types with different access levels

Are these on purpose?

Yes, I used them to debug but then I thought it improved the output for the user

Why remove this Field component? Can't it be used with another form library? Seems quite generic to me

The only function it had was to spread the input prop, which Redux Form provides. If we're not using redux-form, it's not very useful

I'm not particularly fluent in TS but shouldn't this be an object type, rather than any? Don't know if TS requires you to make the object shape explicit (which clearly we cannot know) so in that case this would be fine.

You can specify that something is an object, which is more specific than any. Although we could support having a state which is not an object I don't see why we would do that. So I agree, I'll change it 👍

What happens with public/private routes? Are you delegating the reddirection responsability somewhere?
What I mean is, for example, what if a non-logged user tries to access a private route? We used to handle that kind of cases inside the route component, how do you propose we do that now?
I think the best way would be for this to be included in the component, or at least create a HOC/wrapper to handle these changes.

redirectTo is a function which receives a user and returns a path to redirect in case that user doesn't meet the condition to show that route. This way we could not only avoid logged users to go to the login or non-logged users to go to the home, we can check access levels or anything we'd like from the user. And with small changes (receiving something other than a user) we could also have access to some routes depending on other factors that aren't the user if we wanted to. This component now is more flexible than the previous AuthenticatedRoute.

same

I don't want this service, we weren't even using it. There's a card for GA though: https://trello.com/c/xba9v0SJ/19-google-analytics-seleccionable

why not include the null case type in the interface definition?

I'm not sure what you mean. If you want to allow devs to use and endpoint and avoid handling the error, I don't agree. In every endpoint we should have a possible error and handle it. I think this will force us to do that, which I believe it's an improvement. Error should never be null, endpoints can always fail.
The only reason that it's nullable here is that it starts as null. And I think the definition should not be null (anything could be null in some contexts, so if we make Error nullable we should make anything nullable and it wouldn't make sense)

don't really understand what this does 🤔

1. I removed the request ids, if that's what you meant here
2. withPostSuccess allows the user to do something with the response data at the moment the request finished
3. transformedResponse is a function you can optionally pass to change the shape of the response

I don't understand how you intend to use the transformResponse. I mean, you always mutate data on success, but you re-mutate it here as well. Shouldn't there be 2 different functions here?

I don't understand what you mean. Since both onPostFetch and onSuccess are different callbacks which receive the response, I can't extract the transformed response, so I must do it in these two places separately. withPostFetch is for doing stuff after the request finished, independantly of whether it failed or not

why 5?

I honestly don't know, but it works. This is the tsconfig file for the bootstrap project. It didn't have one and most TS files broke. So I just copied the one from the generated project (which already existed and worked, I think @pabloferro configured it) so the bootstrap doesn't show strange TS errors. We could check this afterwards

Weren't we going to move the switch to a function?

It's really hard to type 😢

I think I told you that in order to have strong action typing I had to use switch. We encountered issues trying to move to a reducerCreator with an object, either because of TS limitations or because we just couldn't figure it out. After a while I decided to go with this as a first approach so as to finish

This is an obligatory div? If not, I think we should delete it

I didn't put this here (I just changed it to TS). Nevertheless, I think it depends on how you look at it. It is not mandatory, but it might be good to have a container for the whole app with the height set to 100vh (or whatever size it should be) so we don't have to do that in every screen's container.

Why though? Why not using type boolean? 🤔

Or just true or false directly

yeah, there's kind o a bug I think in TS with this. The problem is that apisauce has the following definition:

export interface ApiErrorResponse<T> {
  ok: false
  problem: PROBLEM_CODE
  originalError: AxiosError

  data?: T
  status?: number
  headers?: {}
  config?: AxiosRequestConfig
  duration?: number
}
export interface ApiOkResponse<T> {
  ok: true
  problem: null
  originalError: null

  data?: T
  status?: number
  headers?: {}
  config?: AxiosRequestConfig
  duration?: number
}

The issue is that, for instance if I want to have an object with type ApiOkResponse and I set true, it shows the following error:

So I had to cast it to a type with the fixed value of true.

Although I just realized I can do this:
ok: true as true

and it appears to work. I'll change it.

Here's an issue related to this, although I don't understand if they're working on it or not: microsoft/TypeScript#24413

What is this extends any doing?

I'll change it to {}, I was using it to pass as prop type for the wrapped component

Just a suggestion, if we are going to remove Redux and with that redux-form. Shouldn't we add Formik? Maybe we can have it already installed as a customized library

maybe, but I won't do it in this PR. As of right now, nothing is actually using redux-form, so I'm not removing any functionality. We should think if we want to force Formik, set it as optional dependency or what do we actually want

Should we add Formik?

same answer

Don't we need to change this for a global Context Folder.? Just for global values, like Auth

global context is in ~app/context and it's already being added in this same object:

module.exports.FILES = [
  ...
  'src/app/reducer.ts',
  'src/app/context.ts',
  ...
]

If we add a context folder we should add it here

I think the I answered this in the previous comment

I noticed we remove documentation from some components. Should we add a little bit of this component? Just explaining what it does and when to use it

I think all the docs I removed are for components that aren't present anymore. I like the idea of adding documenttion for this component, but I rather do it in another card so we can close this. I added it to the backlog: https://trello.com/c/ficXXNHQ/241-agregar-documentacion-para-withprovider

I know that you are just using this to set an example. But i think it is rather confusing because here you just need a setState. You really don't want to set "React" in all the context. Maybe we can think of a better example and leave this with setState. For example we could dispatch a list of technologies and use them in a children component like a button and show a warning.

I realize it's not the best example, I just wanted to be expeditive. I don't think this is very important though, given that this component will be removed 100% of the times.

I think these 2 should be on a folder call Hooks where you can create your custom hooks. I would also add it to the main src and add an access with ~

components
service
hooks
  | useRequest
     | index.ts
     | constants.ts (in case you need it)
     | test.ts
  | useAsyncRequest
....

Im saying this because we should encourage devs to create their custom hooks which not only is a good practice, it solves lots of problems of code repetition. I think with this we normalize the place to where create them and we already give them 2 examples of how to create custom hooks

I like it, I'll change it

I don't think it is a discussion for now, but should we use ~ or @ in Viajeros we had some problems using ~ as it assumed all of those roots were imported from node modules. In amex we used @ and we could avoid the internal/external package problem using the following eslint configuration.

'import/order': [
      'error', {
        'newlines-between': 'always',
        groups: ['builtin', 'external', ['unknown', 'internal'], 'parent', 'sibling', 'index']
      }
    ]

With this eslint can differenciate between @apollo and @components. I think it could be a good improvment, what do you think?

I think I had issues with jest for some reason when I tried it to use @. Can you add a card in here https://trello.com/b/sZVeiFgw/departamento-de-frontend so we can do this separatedly?

I'd move this to the constants file since it's relevant to the routes definition.

I had to move it elsewhere because it was generating circular dependencies. Routes constant import components and those components sometimes import paths. If those paths are in Routes, that's a circular dependency. Given that components shouldn't import routes, only path, I think it might be actually better this way.

Can you rename the component to match the folder name?

Do you need to export individual action types?

probably not... at least not right now. I'll remove it

It's a good point.
I'm just thinking out loud, but wouldn't it make sense to have different contexts for different global data?
To avoid unnecessary updates, for example updating consumers of the user when the theme changes.

Same as before, do you need to export these?

Or just true or false directly

does that work?

ok: false as false works, I just tested it and I'll change it

This looks great!

Maybe we can name the file requests or something like that instead of utils?

I'll take the suggestion from @LucasZibell and move them to a hooks folder to encourage creating custom hooks. I think I'll separate them too.

      "~hooks/*": ["app/hooks/*"],
      "~contexts/*": ["app/contexts/*"]

I'd move these to app